-
Notifications
You must be signed in to change notification settings - Fork 912
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Fix Bug 723267] Add new projects/calendar page #1466
Conversation
|
||
{% block js %} | ||
{{ js('projects-calendar') }} | ||
<script>window.addEventListener("DOMContentLoaded", setupVersion('{{ LANG }}'), false);</script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove this inline script and call setupVersion
from within calendar.js
. Perhaps just store the {{ LANG }}
in a data attribute on the body if you need to query it.
Since you've moved
|
color: #FFFFFF; | ||
line-height: 1.3; | ||
background-color: #669BE1; | ||
background-image: -moz-linear-gradient(center top , #669BE1 0%, #5784BF 100%); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should drop the -moz prefix, or, depending on how far backwards-compatible we want to be, keep the -moz prefix and add -webkit as well as unprefixed. For this particular page I'd be happy to go with unprefixed only and older browsers can fall back to the solid color.
We also have a mixin for gradients that produces an exhaustive list if you want to be super-thorough, but it's kind of overkill. https://github.com/mozilla/bedrock/blob/master/media/css/sandstone/lib.less#L237
The bottom of the arrow on the button is getting clipped off. Could probably fix it with a min-height on the title and a negative bottom margin to compensate and pull the description back up (or maybe a negative top margin on the desc). The "free download" text is also a little hard to read, at least for my eyes and my screen. I think it was styled for the lighter green button and doesn't have enough contrast on the blue. Maybe go a bit darker, or switch to a light gray? Might need to adjust the text shadow also, depending on what you try. |
background-image: -moz-linear-gradient(center top , #669BE1 0%, #5784BF 100%); | ||
border-radius: 6px 6px 6px 6px; | ||
box-shadow: 0 3px rgba(0, 0, 0, 0.1), 0 -4px rgba(0, 0, 0, 0.1) inset; | ||
color: #FFFFFF; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repeated color here.
And if I'm picking nits, we prefer lowercase hex and shorthand where possible, so #fff instead of #FFFFFF
@alexgibson Just rewrote the whole thing using jquery ajax. It also now works fine in IE10 for me and possibly earlier IE versions, though in any case, as you noted the download button fallback is pretty good so I'm not overly concerned with old IE versions on this page. Thanks for the tip about window.site.platform, and for taking the time to review! @craigcook I've fixed the following:
Thanks for the tips and your time reviewing! |
Looking good in IE10+ 👍 I believe the only way to do a cross domain request in IE <= 9 is to use the MS propriety XDomainRequest object, or else a hack like JSONP. jQuery won't support But still, like you say the link degrades perfectly well. If we aren't too concerned with these older browsers for this particular page, then that's fine with me 🎉 |
versionBox.textContent = 'Lightning ' + $(xml).find('version').text(); | ||
downloadBox.className = downloadBox.className.replace('loading', ''); | ||
|
||
var downloadLink = downloadBox.getAttribute('href');; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's still an extra semi-colon here that can be removed.
Ok, this one is looking good, thanks @Sancus 👍 r+ |
[Fix Bug 723267] Add new projects/calendar page
No description provided.